Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

libs: add gcov framework support #14838

Merged
merged 2 commits into from
Nov 22, 2024
Merged

libs: add gcov framework support #14838

merged 2 commits into from
Nov 22, 2024

Conversation

Gary-Hobson
Copy link
Contributor

Summary

libs: add gcov framework support

In devices without storage media, you can export data to the
command line and then generate the corresponding gcda file

It can save the result output by calling __gcov_info_to_gcda
The usage is similar to:
https://gcc.gnu.org/onlinedocs/gcc/Freestanding-Environments.html#Profiling-and-Test-Coverage-in-Freestanding-Environments

Usage:
./tools/configure.sh qemu-armv7a:nsh
Modify the configuration

+CONFIG_COVERAGE_ALL=y
+CONFIG_COVERAGE_MINI=y
+CONFIG_SYSTEM_GCOV=y

Run:

qemu-system-arm -cpu cortex-a7 -nographic -smp 4 \
     -machine virt,virtualization=off,gic-version=2 \
     -net none -chardev stdio,id=con,mux=on -serial chardev:con \
     -mon chardev=con,mode=readline -kernel ./nuttx/nuttx -semihosting -s | tee gcov.txt
./nuttx/tools/gcov_convert.py -i ./gcov.txt
./nuttx/tools/gcov.sh -t arm-none-eabi-gcov

Impact

Testing

qemu

@Gary-Hobson
gcov: update gcov compilation parameters
18e8fff 
profile-generate is used to generate compilation feedback optimization data, not just code coverage data

It will rely on the toolchain library:
nuttx/libs/libc/misc/lib_utsname.c:94:(.text.uname+0x2c): undefined reference to `__gcov_indirect_call_profiler_v4'
arm-none-eabi-ld: nuttx/libs/libc/misc/lib_utsname.c:113:(.text.uname+0x178): undefined reference to `__gcov_indirect_call'
arm-none-eabi-ld: nuttx/libs/libc/misc/lib_utsname.c:113:(.text.uname+0x188): undefined reference to `__gcov_time_profiler_counter'
arm-none-eabi-ld: nuttx/staging/libc.a(lib_utsname.o):(.data..LPBX0+0x30): undefined reference to `__gcov_merge_time_profile'

Signed-off-by: yinshengkai <yinshengkai@xiaomi.com>
@github-actions github-actions bot added Area: Tooling Arch: arm Issues related to ARM (32-bit) architecture Arch: arm64 Issues related to ARM64 (64-bit) architecture Arch: risc-v Issues related to the RISC-V (32-bit or 64-bit) architecture Arch: simulator Issues related to the SIMulator Arch: tricore Issues related to the TriCore architecture from Infineon Arch: x86_64 Issues related to the x86_64 architecture Area: OS Components OS Components issues Board: simulator Size: L The size of the change in this PR is large labels Nov 18, 2024
@Gary-Hobson Gary-Hobson mentioned this pull request Nov 18, 2024
Merged
@nuttxpr
Copy link

nuttxpr commented Nov 18, 2024

[Experimental Bot, please feedback here]

This PR appears to mostly meet the NuttX requirements, but is lacking in key details. Here's a breakdown:

Strengths:

  • Clear Summary of the "What": The summary explains the purpose of the change (adding gcov support), the affected area (libs), and the mechanism (exporting data to the command line and generating gcda files). The usage example and configuration instructions are helpful.
  • Provides Testing Information: It specifies the target (qemu) and gives basic usage instructions. The inclusion of gcov.txt processing is good.

Weaknesses:

  • Missing Issue References: If this relates to any existing NuttX issues, those should be linked.
  • Incomplete Impact Assessment: The "Impact" section is entirely empty. This is a critical part of the PR. At a minimum, address all the listed impact points (user, build, hardware, documentation, security, compatibility). Even if the answer is "NO", state it explicitly. For example: "Impact on user: NO". If the impact is "YES", explain the impact.
  • Insufficient Testing Detail: While testing information is present, it's minimal.
    • Build Host: Provide specifics about the build host OS, CPU architecture, and compiler version used.
    • Target Details: Be more specific about the qemu configuration (e.g., qemu-system-arm -cpu cortex-a7 ...).
    • Missing "Before" and "After" Logs: The code blocks for "Testing logs before change" and "Testing logs after change" are empty. Include relevant log snippets that demonstrate the functionality before and after your changes. This is crucial for reviewers to understand the impact of the changes. Show that the gcov output is as expected.

Recommendations for Improvement:

  1. Fill out the Impact section completely. This is the most significant deficiency. Consider these points:

    • Impact on build: Does enabling gcov require any build system changes or new dependencies?
    • Impact on hardware: Is this specific to certain architectures?
    • Impact on documentation: Does this require documentation updates? If so, were those updates included in the PR?
    • Impact on security: Does enabling code coverage introduce any security considerations? (Probably not, but it's good to explicitly state this).
    • Impact on compatibility: Does this change break any existing functionality?

  2. Provide detailed build host information. (OS, CPU, Compiler version).

  3. Include relevant "before" and "after" test logs. Demonstrate that gcov is not working before the change and is working after the change. Show some example gcov output.

  4. Link any related NuttX issues.

By addressing these points, the PR will be much more complete and easier for reviewers to evaluate.

acassis
acassis approved these changes Nov 18, 2024
View reviewed changes
Copy link
Contributor

@acassis acassis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice @Gary-Hobson your commit log message included all needed information to let people start to using it.

cederom
cederom approved these changes Nov 18, 2024
View reviewed changes
Copy link
Contributor

@cederom cederom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @Gary-Hobson :-)

xiaoxiang781216
xiaoxiang781216 reviewed Nov 18, 2024
View reviewed changes
libs/libbuiltin/compiler-rt/Make.defs Show resolved Hide resolved
libs/libbuiltin/libgcc/Make.defs Show resolved Hide resolved
libs/libbuiltin/libgcc/CMakeLists.txt Outdated Show resolved Hide resolved
libs/libbuiltin/libgcc/gcov.c Outdated Show resolved Hide resolved
libs/libbuiltin/libgcc/gcov.c Outdated Show resolved Hide resolved
libs/libbuiltin/libgcc/gcov.c Outdated Show resolved Hide resolved
tools/gcov_convert.py Show resolved Hide resolved
anchao
anchao requested changes Nov 19, 2024
View reviewed changes
libs/libbuiltin/libgcc/gcov.c Outdated Show resolved Hide resolved
libs/libbuiltin/libgcc/gcov.c Show resolved Hide resolved
libs/libbuiltin/libgcc/gcov.c Outdated Show resolved Hide resolved
libs/libbuiltin/libgcc/gcov.c Outdated Show resolved Hide resolved
libs/libbuiltin/libgcc/gcov.c Outdated Show resolved Hide resolved
libs/libbuiltin/libgcc/gcov.c Outdated Show resolved Hide resolved
@acassis
Copy link
Contributor

acassis commented Nov 21, 2024

@Gary-Hobson please consider creating later a Guide (https://nuttx.apache.org/docs/latest/guides/index.html) to use GCOV with NuttX using these commands you included in the commit log.

@Gary-Hobson
Copy link
Contributor Author

@Gary-Hobson please consider creating later a Guide (https://nuttx.apache.org/docs/latest/guides/index.html) to use GCOV with NuttX using these commands you included in the commit log.

Yes, we will upload the documentation for checking code coverage in different environments later

Currently, we support gcc, clang, simulator, device, it will take some time to verify all environments

anchao
anchao reviewed Nov 22, 2024
View reviewed changes
libs/libbuiltin/libgcc/gcov.c Outdated Show resolved Hide resolved
libs/libbuiltin/libgcc/gcov.c Outdated Show resolved Hide resolved
@Gary-Hobson
libs: add gcov framework support
753ef37 
In devices without storage media, you can export data to the
command line and then generate the corresponding gcda file

It can save the result output by calling __gcov_info_to_gcda
The usage is similar to:
https://gcc.gnu.org/onlinedocs/gcc/Freestanding-Environments.html#Profiling-and-Test-Coverage-in-Freestanding-Environments

Usage:
 ./tools/configure.sh qemu-armv7a:nsh
Modify the configuration
+CONFIG_COVERAGE_ALL=y
+CONFIG_COVERAGE_MINI=y
+CONFIG_SYSTEM_GCOV=y
Run:
qemu-system-arm -cpu cortex-a7 -nographic -smp 4 \
     -machine virt,virtualization=off,gic-version=2 \
     -net none -chardev stdio,id=con,mux=on -serial chardev:con \
     -mon chardev=con,mode=readline -kernel ./nuttx/nuttx -semihosting -s | tee gcov.txt
./nuttx/tools/gcov_convert.py -i ./gcov.txt
./nuttx/tools/gcov.sh -t arm-none-eabi-gcov

Signed-off-by: yinshengkai <yinshengkai@xiaomi.com>
@xiaoxiang781216 xiaoxiang781216 merged commit 0413d74 into apache:master Nov 22, 2024
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xiaoxiang781216 xiaoxiang781216 xiaoxiang781216 approved these changes

@cederom cederom cederom approved these changes

@anchao anchao anchao approved these changes

@acassis acassis acassis approved these changes

Assignees
No one assigned
Labels
Arch: arm Issues related to ARM (32-bit) architecture Arch: arm64 Issues related to ARM64 (64-bit) architecture Arch: risc-v Issues related to the RISC-V (32-bit or 64-bit) architecture Arch: simulator Issues related to the SIMulator Arch: tricore Issues related to the TriCore architecture from Infineon Arch: x86_64 Issues related to the x86_64 architecture Area: OS Components OS Components issues Area: Tooling Board: simulator Size: L The size of the change in this PR is large
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Gary-Hobson @nuttxpr @acassis @anchao @cederom @xiaoxiang781216